Code Review

This pull request extends the RaBitQ x+y split storage and search mode to the Pyramid index, which was previously exclusive to HGraph. It includes comprehensive documentation updates in both English and Chinese, maps the new split parameters from external configurations, updates the Pyramid index logic to handle base-reordering, and adds robust unit tests. The review feedback suggests improving consistency by using the public constant RABITQ_ERROR_RATE for parsing search parameters, and enhancing the map_rabitq_split_param helper function with type safety checks, query bit validation, and cleaner range checks.

Code Review Summary

This PR exposes split RaBitQ parameters for Pyramid, adding x+y split quantization mode support (previously only available in HGraph). The implementation is solid overall with good test coverage, but there are several issues that need attention.

What this PR does well

• Clean separation of the map_rabitq_split_param() helper in an anonymous namespace
• Thorough input validation with clear error messages for all split parameters
• Good test coverage including happy paths, error paths, and compatibility checks
• Proper handling of the rabitq_one_bit_search default via default_rabitq_one_bit_search_
• The ODescent SQ8 flatten optimization is a smart performance improvement for graph construction

Issues found

[critical] get_distance reads build_flatten_interface_ without synchronization
In odescent_graph_builder.h:122-128, the get_distance method reads build_flatten_interface_ (a raw pointer) while prepare_build_flatten() writes to it from a single thread. However, get_distance is called from parallelized tasks (parallelize_task). If prepare_build_flatten() runs concurrently with distance computations, this is a data race. While the current call site in Build() calls prepare_build_flatten() before parallelization, the design is fragile — a future refactor could introduce a race. Consider making build_flatten_interface_ an std::atomic<FlattenInterface*> or documenting the strict ordering requirement.

[suggestion] rabitq_error_rate default is quiet_NaN — fragile sentinel
In pyramid_zparameters.h:105, rabitq_error_rate defaults to quiet_NaN(). This is used as a sentinel to mean "not set". However, NaN has unusual comparison semantics (NaN != NaN is true, NaN < 0.0 is false), which makes it easy to misuse. Consider using std::optional<float> or a separate has_rabitq_error_rate boolean (matching the pattern already used for has_rabitq_one_bit_search).

[suggestion] prepare_build_flatten silently ignores non-RaBitQ quantizers
In odescent_graph_builder.cpp:76, the early return checks flatten_interface_->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_RABITQ. This means if someone uses SQ8 or FP16 quantizers, the optimization is silently skipped. Consider adding a debug log message when the optimization is skipped to aid troubleshooting.

[note] Missing rabitq_error_rate in CheckAndMappingExternalParam mapping table
The rabitq_error_rate search parameter is parsed in PyramidSearchParameters::FromJson() but is not part of the external_mapping table in CheckAndMappingExternalParam(). This is actually correct since it is a search-time parameter, not a build-time parameter. However, the naming could be confusing — consider adding a comment to clarify the distinction.

[note] Test file pyramid_zparameters_test.cpp uses nlohmann::json directly
The test file mixes vsag::JsonType and raw nlohmann::json in several places (e.g., line 152). While this works, it is inconsistent with the rest of the codebase which exclusively uses vsag::JsonType. Consider using vsag::JsonType consistently for maintainability.

Code Review Summary

Overall Assessment

This PR exposes split RaBitQ parameters for the Pyramid index, allowing x+y split storage mode (previously only available in HGraph). The implementation is well-structured and follows established patterns from the HGraph split RaBitQ integration.

Strengths

• has_precise_reorder() / get_reorder_codes() helpers in pyramid.h correctly distinguish split mode (reorder from base codes) from traditional reorder (separate precise codes), avoiding confusion between the two paths.
• Parameter validation in map_rabitq_split_param() is thorough: filter_bits ∈ [1,8], supplement_bits ∈ [1,8], total ≤ 8, query_bits == 32.
• prepare_build_flatten() in odescent_graph_builder.cpp properly handles the build-time distance computation gap when quantizer is RaBitQ.
• Test coverage added for split RaBitQ builds with ODescent in both integration and unit tests.
• Documentation updated across 6 files (EN/ZH) with parameter tables and configuration examples.

Issues Identified (7 inline comments posted)

Recommendation

The core implementation is solid. Items 1-3 are minor suggestions that could be addressed in this PR. Items 4-6 are P2 follow-ups suitable for post-merge. Overall the PR is in good shape for merging.

Code Review Summary

This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.

Overall Assessment

The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.

Key Observations

1. map_rabitq_split_param ordering (pyramid.cpp:1089): Called after mapping_external_param_to_inner, correctly overwriting the previously-mapped rabitq_bits_per_dim_base with total_bits (x+y). A comment noting this intentional overwrite would help future readers.

2. RABITQ_ERROR_RATE dual mapping (pyramid.cpp:1048-1051): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging.

3. Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.

Recommendation

Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.

Code Review Summary

This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.

Overall Assessment

The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.

Key Observations

1. map_rabitq_split_param ordering (pyramid.cpp:1089): Called after mapping_external_param_to_inner, correctly overwriting the previously-mapped rabitq_bits_per_dim_base with total_bits (x+y). A comment noting this intentional overwrite would help future readers.

2. RABITQ_ERROR_RATE dual mapping (pyramid.cpp:1048-1051): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging.

3. Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.

Recommendation

Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.

Code review complete. Two inline comments posted. Summary: approve with minor notes on parameter mapping ordering and error rate dual mapping.

Approve with minor notes.

Code Review Summary

This PR adds RaBitQ x+y split parameter support to the Pyramid index, enabling split storage and reordering where the base codes serve as both filter codes (x bits) and reorder source (x+y bits), eliminating the need for a separate precise codes store.

Overall Assessment

The implementation is well-structured and follows existing patterns from the HGraph split RaBitQ integration. The parameter mapping, validation, and build-time SQ8 flatten interface are correctly implemented. Test coverage is thorough with unit tests for parameter parsing, functional tests for build and search with split mode, and integration tests for the full pipeline.

Key Observations

1. map_rabitq_split_param ordering (pyramid.cpp): Called after mapping_external_param_to_inner, correctly overwriting the previously-mapped rabitq_bits_per_dim_base with total_bits (x+y). A comment noting this intentional overwrite would help future readers.

2. RABITQ_ERROR_RATE dual mapping (pyramid.cpp): Mapped to both base and precise codes quantization params. In non-split mode where precise_codes uses fp32, the key is harmlessly ignored, but it may confuse debugging.

3. Most previously-raised concerns have been addressed: String type checks, query bits validation, split NSW promotion fix, and thread-safety documentation have all been incorporated by the author.

Recommendation

Approve. The remaining notes are non-blocking clarity improvements. The core logic is correct and well-tested.

Automated inline review completed.

Review effort: high (707 changed lines across 20 files).
Submitted 2 inline comments.

Reviewed commit 3c23e39.

Code Review Summary

This PR exposes RaBitQ x+y split parameters for Pyramid, enabling split storage and lower-bound search. The implementation is generally well-structured with good validation and test coverage. Below are my findings after a thorough review.

What this PR does well

• Clean separation of split parameter mapping into map_rabitq_split_param() with thorough validation
• has_precise_reorder() / get_reorder_codes() helpers correctly handle the split case where precise_codes_ is null and reorder uses base_codes_
• SupportSplitCodeStorage() virtual method provides a clean extension point on FlattenInterface
• Good test coverage in pyramid_zparameters_test.cpp for both valid and invalid split parameter combinations
• The SQ8 build-time distance optimization for ODescent is a nice performance improvement for split RaBitQ

Issues found

[suggestion] PYRAMID_RABITQ_BITS_PER_DIM_PRECISE maps to PRECISE_CODES_KEY → RABITQ_QUANTIZATION_BITS_PER_DIM_BASE_KEY in CheckAndMappingExternalParam, but this mapping is dead code for the split path

In pyramid.cpp:1185-1186, the external key PYRAMID_RABITQ_BITS_PER_DIM_PRECISE is mapped to {PRECISE_CODES_KEY, QUANTIZATION_PARAMS_KEY, RABITQ_QUANTIZATION_BITS_PER_DIM_BASE_KEY}. However, map_rabitq_split_param runs after mapping_external_param_to_inner and overwrites the base codes rabitq_bits_per_dim_base to total_bits (x+y). Meanwhile, PyramidParameters::FromJson sets precise_codes_param = nullptr for split mode, so the precise codes quantization params are never consumed. This mapping is harmless but misleading — it suggests the precise bits are used somewhere in the precise codes path when they are not. Consider removing this mapping entry or adding a comment explaining it is overridden by map_rabitq_split_param.

[suggestion] build_by_odescent creates redundant SQ8 instances in multi-hierarchy mode

In pyramid.cpp:319-326, when thread_pool_ != nullptr && hierarchies_.size() > 1, each hierarchy creates its own ODescent instance. Each ODescent::Build() calls prepare_build_flatten(), which creates a separate SQ8 FlattenInterface — duplicating the same training and insertion work N times for N hierarchies. Since all hierarchies share the same raw vectors, the SQ8 flatten could be created once and shared. The current code partially addresses this by calling CreateBuildFlatten once outside the loop (line 320), but then passes it to each ODescent constructor which will still call prepare_build_flatten() → CreateBuildFlatten() again (the early return on build_flatten_interface_ != nullptr prevents the second creation, but the first creation inside prepare_build_flatten still happens for the first hierarchy). Consider passing the pre-created build_flatten directly to each ODescent constructor to avoid the redundant prepare_build_flatten call.

[note] reorder_source uses HGraph-specific constants in Pyramid code

In pyramid_zparameters.cpp:171-172, reorder_source is set to HGRAPH_REORDER_SOURCE_BASE or HGRAPH_REORDER_SOURCE_PRECISE. These are HGraph-specific constants (defined in include/vsag/constants.h). While functionally correct since the values are just strings ("base"/"precise"), this creates a conceptual coupling between Pyramid and HGraph. If these constants are ever renamed or their semantics change for HGraph, Pyramid would be affected. Consider defining Pyramid-specific constants or using the raw string values directly.

[note] rabitq_error_rate uses quiet_NaN() as sentinel — consider std::optional<float>

In pyramid_zparameters.h:105, rabitq_error_rate defaults to std::numeric_limits<float>::quiet_NaN(). NaN has unusual comparison behavior which can lead to subtle bugs. The has_rabitq_one_bit_search / rabitq_one_bit_search pair already demonstrates a cleaner pattern. Consider using std::optional<float> or adding a has_rabitq_error_rate boolean to match the existing convention.

[note] search_impl uses use_reorder_ directly while most other sites use has_precise_reorder() — correct but inconsistent

In pyramid.cpp:538, search_impl checks use_reorder_ directly for the reorder step, while most other sites in this PR use has_precise_reorder(). This is correct — the reorder should run in split mode too (using base_codes_ via get_reorder_codes()). However, the inconsistency with the rest of the refactoring could confuse future readers. Consider adding a brief comment explaining why use_reorder_ is used here instead of has_precise_reorder().

Review Summary for commit 3c23e395 ("fix(pyramid): complete split RaBitQ integration")

This commit addresses several prior review comments and completes the split RaBitQ integration for Pyramid. The key changes:

Positive findings:

• SupportSplitCodeStorage() virtual method is a clean refactoring over the previous reorder_by_base_ member variable. It eliminates the risk of the flag and data cell type drifting out of sync.
• map_rabitq_split_param now properly validates that use_reorder=true when split params are present, rejecting invalid configurations early.
• CreateBuildFlatten correctly gates on SupportSplitCodeStorage(), so non-split quantizers get nullptr and get_distance() falls back to flatten_interface_.
• The test now uses 3+5 split bits (more realistic than 1+1) and verifies reorder_lower_bound_probe_count > 0 for split mode.
• pyramid_zparameters_test.cpp adds a test case for use_reorder: false with split params (expects throw).

Remaining items (all previously noted):

• The lower-bound collection logic is duplicated between KnnSearch and RangeSearch (lines 395-417 and 467-489). Consider extracting into a helper.
• ctx.rabitq_error_rate is set but not yet consumed by the search path (known P2 follow-up).
• default_rabitq_one_bit_search_ defaults to true for split indexes, but lower-bound candidates are not yet fully propagated through reorder.

Overall: The refactoring is solid and addresses the most important prior feedback. The remaining items are non-blocking follow-ups that don't affect correctness.

lgtm

LGTM